Fixes #5358. Narrow draw pipeline to skip parent on child-only redraws#5431
Merged
harder merged 14 commits intoMay 28, 2026
Merged
Conversation
…n-out Capture NeedsDraw at the top of View.Draw() before DoDrawAdornments mutates NeedsDrawRect, and gate DoClearViewport / DoDrawText / DoDrawContent on the captured value. When a parent enters Draw() only because SubViewNeedsDraw is true, its viewport is no longer cleared and its own text/content is no longer redrawn. Adornments, subviews, and line canvas still render correctly. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The escalation remains necessary for DoRenderLineCanvas and ClearNeedsDraw, but it no longer drives parent self-content redraws — those are now gated on the needsDrawSelf snapshot captured before this escalation. Updated the inline comment to reflect the narrowed purpose. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing draw Remove the redundant SetNeedsDraw() call in the base AdornmentView's OnClearingViewport. The thickness has just been drawn; calling SetNeedsDraw mid-pass cascades to the parent's SubViewNeedsDraw and adds churn that competes with the needsDrawSelf gate introduced for gui-cs#5358. MarginView already overrides without this call. BorderView and PaddingView, which inherit the base implementation, also benefit. Also refresh the stale class doc comment — Border and Padding now extend AdornmentView too. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five tests covering the needsDrawSelf gate: - ChildOnlyDirty: parent does not clear/redraw self - ParentDirectlyDirty: full redraw path still works (regression guard) - TransparentParent: transparent early-return composes with the gate - ParentWithBorder: parent skips clear/text even with an adornment present - IOutput layer: child's render still reaches IOutput after the fix Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-cs#5358 DrawComplete fires unconditionally for clip-exclusion bookkeeping even when NeedsDraw is false, so it is the wrong signal for measuring whether a view actually did draw work. Add a DrawingText counter to both the diagnostic and integration tests — DrawingText is gated on NeedsDraw inside DoDrawText and is not intercepted by Code (which does intercept ClearedViewport / DrawingContent via OnClearingViewport / OnDrawingContent returning true). Use DrawingText to verify per-Code inactive-tab behavior, and use tabsCounts.ClearedViewport to verify the parent's own behavior. TabsFanOutDiagnosticTests (synthetic Layout/Draw path) now asserts: - inactive tabs have DrawingText == 0 - Tabs container has ClearedViewport == 0 - text-draw fan-out ratio == 1.0 (parity with single-Code baseline) TabsFanOutIntegrationTests (end-to-end via main loop) keeps the existing "documents remaining fan-out" assertion: a separate cascade source remains in ApplicationImpl.LayoutAndDraw (force=true when neededLayout, calling SetNeedsDraw on the top runnable which cascades to overlapping subviews). Removing that force=true uncovers stale-content bugs in the shrink/move path (already covered by ShadowTests / BorderViewTests) and is out of scope for gui-cs#5358. Layout fan-out is fully eliminated by PR gui-cs#5373. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous union math at View.NeedsDraw.cs lines 87-92 was buggy: int x = Math.Min (Viewport.X, viewPortRelativeRegion.X); int y = Math.Min (Viewport.Y, viewPortRelativeRegion.Y); int w = Math.Max (Viewport.Width, viewPortRelativeRegion.Width); int h = Math.Max (Viewport.Height, viewPortRelativeRegion.Height); It unioned against Viewport (not against the existing NeedsDrawRect), and used max-of-Width / max-of-Height rather than Rectangle.Union. The result was that NeedsDrawRect was effectively clamped to the current Viewport size on every call — accumulated partial regions were silently widened to ~viewport-wide, making NeedsDrawRect useless for narrowing draw work. Replace with Rectangle.Union (NeedsDrawRect, viewPortRelativeRegion) so NeedsDrawRect accurately tracks the accumulated dirty area. This is required for issue gui-cs#5358's region-aware ClearViewport. Update NeedsDrawTests to reflect the new accumulating semantics: - NeedsDrawRect_Is_Viewport_Relative: shrink scenarios now keep the larger accumulated area (Union of prior larger frame and current smaller one) - SetNeedsDraw_MultipleRectangles_Expands: union of two non-overlapping rects is the bounding rect, not the full viewport Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When NeedsDrawRect is set and genuinely smaller than the full viewport (i.e. a real partial-region invalidation, not SetNeedsDraw()-no-arg which sets the full Viewport), narrow the clear and the trailing SetNeedsDraw cascade to just that region. Otherwise preserve the existing full-clear contract used by Code.OnClearingViewport, MarkdownCodeBlock, and direct test callers (e.g. ClearViewport_FillsViewportArea, ClearViewport_SetsNeedsDraw). This is the second half of the infrastructure for issue gui-cs#5358's narrowed draw work: combined with the SuperView invalidation on Frame change added in the next commit, geometry changes will only clear and cascade to the affected region instead of forcing the whole viewport and all overlapping peers to redraw. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a view's Frame shrinks or moves, the SuperView's old-frame area is now uncovered and must be cleared on the next draw — otherwise stale content remains (e.g. an old shadow after a view shrinks). Add a SuperView.SetNeedsDraw (Rectangle.Union(originalFrame, Frame)) call in View.Layout(Size) when frameChanged. Combined with the region-aware ClearViewport from the previous commit, this clears only the union area and cascades NeedsDraw only to overlapping subviews intersecting that union — not the entire viewport. Note: this only catches Frame changes. Adornment thickness changes (e.g. a Margin shrinking when a shadow is removed) don't change Frame, so this fix doesn't help them. A future change can extend this by tracking adornment thickness deltas during Layout — at which point ApplicationImpl.LayoutAndDraw can drop force=true on neededLayout and the integration-level draw fan-out in TabsFanOutIntegrationTests can be flipped to assert == 0. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This is the cascade source that keeps TabsFanOutIntegrationTests bandaged. Removing force=true when only neededLayout (not forceRedraw) requires tracking adornment thickness changes so the SuperView can be invalidated precisely. Until that lands, ShadowTests and BorderViewTests rely on force=true to clear stale content after adornment thickness rebalances that don't change Frame. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tests SetFrame is the direct-assignment path for view.Frame = ... (used by code that sets Frame outright). It bypasses Layout()'s frameChanged detection because SetFrame updates _frame before Layout runs. Add the same SuperView.SetNeedsDraw (Union(oldFrame, newFrame)) call in SetFrame so direct Frame assignments also trigger the region-aware uncovered-area clearing. Add RegionAwareClearViewportTests with 5 focused tests: - DirectCaller_EmptyNeedsDrawRect_ClearsFullViewport — backward-compat contract - PartialNeedsDrawRect_ClearsOnlyDirtyRegion — narrowing fires when partial - FullViewportNeedsDrawRect_ClearsFullViewport — SetNeedsDraw() no-arg path - FrameShrink_InvalidatesSuperViewWithUnionOfOldAndNewFrames — Step 3 catches it - Scroll_DoesNotInvalidateSuperView — pure scroll does not fan out Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…serve transparent cache CoPilot code review surfaced three correctness issues with the region-aware ClearViewport from the prior commit: 1. Coordinate-space inconsistency. SetNeedsDraw(Rectangle) cascades to subviews using frame-local coordinates (subtracts subview.Frame.X/Y), while the no-arg variant passes content-coord Viewport. For an unscrolled view those coincide; for a scrolled view they disagree, and my narrowing math (subtracting Viewport.X/Y) would shift the clear off-screen on a scrolled subview. 2. Code/MarkdownCodeBlock full-clear contract. Both call public ClearViewport() from their OnClearingViewport overrides expecting a full fill of the code-block background. With NeedsDrawRect set partial by the cascade, the narrowed clear left stale background in the un-cleared part of the viewport (visible when content has fewer lines than the viewport, etc.). Fix: revert public ClearViewport() to always full-clear. Move narrowing logic into a private CanNarrowClearToNeedsDrawRect helper invoked only from DoClearViewport (the framework's draw-pipeline entry point), guarded by Viewport.Location == Point.Empty so the scrolled-cascade ambiguity is sidestepped entirely. The Item 1 normalization (frame-local vs content-coord NeedsDrawRect) is a deeper refactor deferred as a follow-up. 3. TransparentMouse hit-region wipe. _localDrawContext was recreated unconditionally before the needsDrawSelf gate, but on child-only passes DoDrawText/DoDrawContent are skipped, so _localDrawContext stayed empty. DoDrawComplete then wrote that empty region into CachedDrawnRegion, breaking mouse hit-testing for transparent views until the next full self-redraw. Fix: only recreate _localDrawContext when needsDrawSelf is true. On child-only passes the prior context is preserved, so DoDrawComplete keeps the previously- cached drawn region. SetNeedsDraw nulls CachedDrawnRegion when the view is genuinely invalidated, so stale cache values are bounded by view lifecycle. Updated tests: - RegionAwareClearViewportTests: switched from testing the public ClearViewport API to testing the framework's DoClearViewport via Draw(). Added a FrameworkDraw_ScrolledView_FallsBackToFullClear test that locks in the scrolled-view guard from fix 1. - SubViewOnlyRedrawTests: added TransparentMouseParent_ChildOnlyDirty_ PreservesCachedDrawnRegion to lock in fix 3. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CoPilot review item 4: SetFrame() already calls SuperView.SetNeedsDraw(union) for every Frame mutation (both direct view.Frame=... and layout-driven paths go through SetFrame). The matching block in Layout(Size) repeated the union and the cascade work on the hot path this PR is trying to trim. The duplicate was idempotent (Rectangle.Union of identical rects produces the same rect), but the cascade-to-overlapping-subviews work ran twice. Refs gui-cs#5358 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR narrows the draw pipeline so child-only redraws no longer cause parent viewport clearing or parent text/content redraws, addressing issue #5358 while preserving adornment, line-canvas, and public ClearViewport() behavior.
Changes:
- Gates parent self-drawing work on a pre-adornment
NeedsDrawsnapshot. - Fixes dirty-region union semantics and adds region-aware framework clearing.
- Adds/updates regression tests for child-only redraws, tab fan-out diagnostics, frame-change invalidation, and remaining integration-level fan-out.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
Terminal.Gui/ViewBase/View.Drawing.cs |
Adds self-redraw gating and region-aware DoClearViewport narrowing. |
Terminal.Gui/ViewBase/View.Drawing.Adornments.cs |
Updates comments around adornment-driven NeedsDrawRect escalation. |
Terminal.Gui/ViewBase/View.NeedsDraw.cs |
Replaces broken dirty-rect expansion with Rectangle.Union. |
Terminal.Gui/ViewBase/View.Layout.cs |
Invalidates the SuperView for old/new frame union on frame changes. |
Terminal.Gui/ViewBase/Adornment/AdornmentView.cs |
Removes redundant mid-clear SetNeedsDraw() cascade and updates docs. |
Terminal.Gui/App/ApplicationImpl.Screen.cs |
Documents the remaining LayoutAndDraw force-redraw fan-out source. |
Tests/UnitTestsParallelizable/ViewBase/Draw/SubViewOnlyRedrawTests.cs |
Adds focused child-only redraw regression coverage. |
Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs |
Adds coverage for partial clears, full-clear contracts, and frame/scroll cases. |
Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs |
Updates expectations for true dirty-region union behavior. |
Tests/UnitTestsParallelizable/Views/TabView/TabsFanOutDiagnosticTests.cs |
Switches diagnostics to DrawingText and asserts inactive tabs avoid NeedsDraw-gated work. |
Tests/IntegrationTests/TabsFanOutIntegrationTests.cs |
Tracks DrawingText and documents remaining integration-level fan-out. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
tig
approved these changes
May 28, 2026
Member
tig
left a comment
There was a problem hiding this comment.
This is really good stuff. Challenging. So glad we have the AIs to help.
harder
added a commit
to harder/Terminal.Gui
that referenced
this pull request
May 28, 2026
NeedsDrawRect and the viewPortRelativeRegion parameter of SetNeedsDraw are now in viewport-LOCAL coords: (0, 0) is the top-left visible cell of the View's Viewport, independent of Viewport.Location (scroll offset, possibly negative under AllowNegativeX/Y). Previously the no-arg SetNeedsDraw() passed Viewport (with the scroll offset baked in), DoDrawAdornments escalation did the same, and the subview cascade compared parent's viewport-relative region to subview.Frame (in parent's content coords) without accounting for the parent's scroll. PR gui-cs#5431 worked around the resulting inconsistency by gating its region-aware ClearViewport narrowing on "Viewport.Location == Point.Empty"; this commit normalizes the convention so that workaround can be removed in a follow-up commit. Changes: - SetNeedsDraw() no-arg now passes new (Point.Empty, Viewport.Size). - DoDrawAdornments NeedsDrawRect escalation does the same. - SetNeedsDraw(Rectangle) subview cascade translates the viewport-local region through: parent scroll -> content coords -> intersect with subview.Frame -> subview frame-local -> subtract subview adornment offset and subview scroll -> clip to subview viewport bounds. If the result is empty (dirty area is only in subview's adornments/scrolled- off content), fall back to a no-arg subview SetNeedsDraw so the subview redraws safely. - View.Layout.SetFrame translates the union(old, new) frame from SuperView content coords to SuperView viewport-local before passing to SuperView.SetNeedsDraw. - Update NeedsDrawRect_Is_Viewport_Relative test scrolled-frame assertions to reflect the new convention (NeedsDrawRect no longer acquires scroll-position offsets). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
harder
added a commit
to harder/Terminal.Gui
that referenced
this pull request
May 28, 2026
…e normalized PR gui-cs#5431 added a "Viewport.Location == Point.Empty" early-return to CanNarrowClearToNeedsDrawRect because the cascade in SetNeedsDraw produced rects in a different coordinate space than ViewportToScreen expects. Issue gui-cs#5359 normalized NeedsDrawRect on viewport-local coordinates, so the conversion via ViewportToScreen is now safe regardless of the view's scroll state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
harder
added a commit
to harder/Terminal.Gui
that referenced
this pull request
May 29, 2026
Extract the self-content draw (Text + Content + local-context merge) into a private DrawSelfContent helper that owns the full _localDrawContext lifecycle for a self-redraw pass: create, populate, merge. Move the context creation out of the viewport-clear block — DoClearViewport only writes the shared context, so the local context isn't needed until DrawSelfContent, keeping create/populate /merge in one place. This replaces three separate `if (needsDrawSelf)` blocks (whose alignment had to be maintained by hand) with a clear/self split documented as intentional around DoDrawSubViews, plus an authoritative lifecycle comment on the _localDrawContext field describing why it is preserved across child-only passes (TransparentMouse CachedDrawnRegion contract from gui-cs#5431). Behavior-preserving; draw order unchanged. Side effect: collapsing creation and use into one method lets nullable flow analysis prove _localDrawContext non-null, removing a CS8602 warning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
This is awesome thank you. |
6 tasks
This was referenced Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stops the parent's
Draw()pipeline from clearing the viewport and redrawing its own text/content when only a subview is dirty. Adds the supporting infrastructure (true-unionSetNeedsDraw, region-awareClearViewport, SuperView invalidation on Frame change) so future work can also narrow the remaining fan-out source inApplicationImpl.LayoutAndDraw.Split from #4973. Layout fan-out was already fixed by #5373 (PR for #5357); this PR addresses the draw-pipeline half.
Root causes (verified against current
develop)DoDrawAdornmentsescalatesNeedsDrawRect = Viewportwhen it was empty, which makes downstreamDoClearViewport/DoDrawText/DoDrawContentrun on every parent that was entered throughSubViewNeedsDraw.ClearViewportcallsSetNeedsDraw()at the end — which cascades to every overlapping subview because the existing cascade inView.NeedsDraw.csinvalidates all intersecting children.AdornmentView.OnClearingViewportcallsSetNeedsDraw()mid-pass, propagating churn back up to the parent'sSubViewNeedsDraw.SetNeedsDraw(Rectangle)union math was buggy — it unioned againstViewport(notNeedsDrawRect) and usedmax(width, width)instead ofRectangle.Union, soNeedsDrawRectwas effectively clamped to ~viewport-wide on every call. This made any region-aware narrowing impossible.Changes
View.Draw()(View.Drawing.cs) — capturesneedsDrawSelf = NeedsDrawbeforeDoDrawAdornmentsescalatesNeedsDrawRect; gatesDoClearViewport,DoDrawText,DoDrawContenton the captured value.DoDrawAdornments,DoDrawSubViews,DoDrawAdornmentsSubViews, andDoRenderLineCanvasstill run as before so adornments and line-canvas joining keep working.SetNeedsDraw(Rectangle)(View.NeedsDraw.cs) — replaces the buggy union math withRectangle.Union(NeedsDrawRect, viewPortRelativeRegion)soNeedsDrawRectaccurately tracks accumulated dirty regions.DoClearViewport/ClearViewport(View.Drawing.cs) — publicClearViewport()keeps its original always-full-clear contract (preservesCode.OnClearingViewport,MarkdownCodeBlock,ClearViewport_FillsViewportArea,ClearViewport_SetsNeedsDrawsemantics). Region-aware narrowing lives in a new privateCanNarrowClearToNeedsDrawRecthelper that the framework'sDoClearViewportcalls when (a)NeedsDrawRectis strictly smaller than the viewport AND (b) the view is not itself scrolled (Viewport.Location == Point.Empty). The "not itself scrolled" guard sidesteps a separateNeedsDrawRectcoordinate-system ambiguity (the cascade inView.NeedsDraw.csproduces frame-local rects whileSetNeedsDraw()-no-arg passes content-coordViewport); normalizing that convention is deferred.SetFrame(View.Layout.cs) — when a view'sFrameshrinks or moves, callsSuperView.SetNeedsDraw(Rectangle.Union(originalFrame, Frame))so the uncovered area gets cleared on the next draw via the framework's region-aware path.SetFrameis the single source of truth for_framemutation: bothview.Frame = ...and the layout-drivenSetRelativeLayout→SetFramepaths go through it.View.Draw()-_localDrawContextlifecycle (View.Drawing.cs) — only recreates the per-pass draw context whenneedsDrawSelfis true. On child-only passes the prior context is preserved, soDoDrawCompletedoesn't overwrite a transparent view'sCachedDrawnRegionwith an empty region. PreventsTransparentMousehit-testing from going dead after a child-only invalidation.AdornmentView.OnClearingViewport(AdornmentView.cs) — removes the redundantSetNeedsDraw()call that cascaded back to the parent'sSubViewNeedsDrawmid-draw.MarginViewalready overrides without it;BorderViewandPaddingView(both: AdornmentView) inherit the cleaner base. Also refreshes the stale migration doc comment that still claimed Border and Padding extendedAdornmentdirectly.Testing
dotnet build --no-restoredotnet run --project Tests/UnitTestsParallelizable --no-build— 17276 tests pass / 17 pre-existing skipsdotnet run --project Tests/UnitTests.NonParallelizable --no-build— 74 pass / 2 skipsdotnet run --project Tests/IntegrationTests --no-build— 433 passNew tests:
Tests/UnitTestsParallelizable/ViewBase/Draw/SubViewOnlyRedrawTests.cs(6 tests) — child-only invalidation does not clear or redraw the parent; transparent and bordered cases; IOutput-layer verification; transparent-mouse parent preserves CachedDrawnRegion across child-only passes.Tests/UnitTestsParallelizable/ViewBase/Draw/RegionAwareClearViewportTests.cs(6 tests) — narrow-when-partial vs. full-clear-when-empty contract; Frame-change invalidates SuperView for the union of old and new frames; pure scroll does not invalidate SuperView; scrolled-view falls back to full clear.Updated tests:
Tests/UnitTestsParallelizable/Views/TabView/TabsFanOutDiagnosticTests.cs— switched fromDrawComplete(fires unconditionally for clip-exclusion bookkeeping) toDrawingText(gated onNeedsDraw, not intercepted byCode). Diagnostic-level inactive-tab fan-out now asserts== 0.Tests/UnitTestsParallelizable/ViewBase/Draw/NeedsDrawTests.cs—NeedsDrawRect_Is_Viewport_RelativeandSetNeedsDraw_MultipleRectangles_Expandsupdated to reflect proper accumulating union semantics (the old assertions were locking in the buggy "max-of-Viewport" behavior).Tests/IntegrationTests/TabsFanOutIntegrationTests.cs— assertion message updated to point at the documented remaining cascade source (ApplicationImpl.LayoutAndDraw'sforce=trueonneededLayout), which is out of scope for this PR.Known remaining work (deferred to a follow-up)
#5434 Track adornment thickness deltas during layout so
ApplicationImpl.LayoutAndDrawcan stop passingforce = neededLayout || forceRedraw; once that lands,TabsFanOutIntegrationTestscan flip to assert integration fan-out== 0.#5433 Make the
View.Draw (...)self-draw /_localDrawContextflow explicit so theTransparentMousecached-region contract stays behavior-safe and easier to maintain.Code-review changes (CoPilot review of initial commits)
Four issues found in code review and addressed in commits
7479c2ddaandac2d48ffa:Viewport.Location == Point.Empty; full convention normalization deferred.CachedDrawnRegionwiped on child-only passes because_localDrawContextwas recreated unconditionally. Fix: only recreate whenneedsDrawSelfis true.Code/MarkdownCodeBlockfull-clear contract broken by partial-narrowClearViewport()(same root cause as 1). Fix: revert publicClearViewportto always-full-clear; narrowing only in the framework path.SetFrameandLayout(Size). Fix: keep onlySetFrame's — single source of truth.To pull down this PR locally